improved layout of verification modal - #950
Conversation
kevinaboos
left a comment
kevinaboos
left a comment
There was a problem hiding this comment.
Hi @jenniferhmartinezmejia-netizen, thanks for your contribution!
I left some comments below. While things generally look fine, I get the sense that you basically just copy-pasted Element's design decisions into Robrix. Please don't just directly copy Element; we're not trying to make an Element clone here, and I don't want to get accused of IP theft.
The only thing we need to actually change here in Robrix is the emoji layout. Nothing else -- the button order, messages shown in the modal body, text phrasing, etc doesn't need to change.
Please disclose what AI Agents or LLMs you used to generate this PR, and whether you manually reviewed each line. If not, please do so as part of your next commit.
|
Hey Kevin! Thank you for reviewing the changes closely. I will make sure to review the comments and make the changes later this week when I have time. I didn't mean to copy Element, apologies for that. I was just trying to follow the comments made by matlu on the issue. That's my bad for not double-checking if you wanted those changes or not. Have a nice rest of your day! |
|
all good, thanks! |
kevinaboos
left a comment
kevinaboos
left a comment
There was a problem hiding this comment.
thanks for addressing most of the issues I raised. Code is generally looking good, but I left a few more questions for new things that I noticed.
kevinaboos
added
the
waiting-on-author
|
hi jennifer, i went ahead and made the requested changes since it's been a while since i last heard from you, and i'd like to include this in the next release of robrix, which is coming soon. Thanks again for your original contribution! |
2 participants
Note: a PR template is required for the CodePath class I am taking. I couldn't find a template in the repo so I used the one provided in the class.
What does this PR do?
Replaces the plain text list of SAS verification emojis in the verification modal with a right-wrapping grid of cells, each displaying a large emoji glyph above its description.
Why was this PR needed?
Users reported the emojis were too small to compare against another device. The root cause was that all seven emojis were concatenated into a single ModalBody label at body-text size (11.5pt), with no per-emoji layout. Now that Makepad supports well-aligned right-wrap flow layouts, each emoji can be rendered as an independent cell at a larger size. See #846.
What are the relevant issue numbers?
Closes #846
Screenshots
Does this PR meet the acceptance criteria?
Project builds and tested locally, (screenshots above). Used Claude to review code, manually reviewed each line.